-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
E2E transfer and command update #17156
base: edge
Are you sure you want to change the base?
Conversation
…will start to work on RQA-3792 to ensure all checkboxes are mantained for single-channel
…y we can do testing. I also updated clickCreateNew to be compatible with 8.2.2 and beyond as well as check settings
…ke an add lawbare and an add liquid function.
This is bringing in edge. Hope it doesn't delete all of my stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!! a couple of initial pointers:
- we have rules for how to name our PRs, i think this one would be a
test(protocol-designer): some description
https://opentrons.slack.com/archives/CMGV77JP4/p1656442016461519 - you can run
make check-js
andmake lint-js quiet=true
to run the lint and check tests locally so you can see what is failing without having to push commits - you'll probably want to rebase/merge edge into this branch so you're up to date! i merged the 8.2.2 release branch into edge earlier today
Will continue reviewing but lmk if you need help resolving any of the errors
cy.contains('Tip handling') | ||
cy.contains('Tip drop location') | ||
|
||
//cy.get('button[type="submit"]').contains('Save').click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are all these comments needed from lines 167 to 286?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, I'll delete them!
It didn't seem to like opentrons/protocol-designer/cypress/support/commands.ts Cypress.Commands.add('chooseDeckSlot', (slot: string) => {
// Define the deckSlots object with a Record type for valid keys
const deckSlots: Record<
| 'A1'
| 'A2'
| 'A3'
| 'B1'
| 'B2'
| 'B3'
| 'C1'
| 'C2'
| 'C3'
| 'D1'
| 'D2'
| 'D3',
() => void
> = {
A1: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'),
A2: () => cy.contains('foreignObject[x="164"][y="321"]', 'Edit slot'),
A3: () => cy.contains('foreignObject[x="328"][y="321"]', 'Edit slot'),
B1: () => cy.contains('foreignObject[x="0"][y="214"]', 'Edit slot'),
B2: () => cy.contains('foreignObject[x="164"][y="214"]', 'Edit slot'),
B3: () => cy.contains('foreignObject[x="328"][y="214"]', 'Edit slot'),
C1: () => cy.contains('foreignObject[x="0"][y="107"]', 'Edit slot'),
C2: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'),
C3: () => cy.contains('foreignObject[x="328"][y="107"]', 'Edit slot'),
D1: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'),
D2: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'),
D3: () => cy.contains('foreignObject[x="328"][y="0"]', 'Edit slot'),
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, please don't be discouraged by all the comments—you're doing an amazing job! 🎉
That said, we need to follow the framework established in protocol-designer/cypress/support/createNew.ts
.
Key Points
-
Commands Structure:
- The
commands
file is intended to house only the top-level landing and navigation code.
- The
-
Starting Point for Tests:
createNew
is our starting place for all tests that progress into the "create new" flow.- If needed, we can rename this for clarity.
-
Example to Follow:
- Refer to
protocol-designer/cypress/e2e/createNew.cy.ts
to see how to structure tests using this pattern:- Create an array of steps.
- Run that array of steps with
runCreateTest(steps)
.
- Refer to
-
Actions and Verifications:
- All app interactions fall into two categories:
- Action: Perform an action in the UI.
- Verification: Validate an expected outcome.
- The implementation of actions and verifications should be centralized for workflows that align with tests.
- For now, everything down the "create new" path should reside in
createNew.ts
.
- All app interactions fall into two categories:
-
Readable Test Files:
- The actual
*.cy.ts
files should read like a clear list of steps likecreateNew.cy.ts
- The actual
-
Scalability:
- While
createNew.ts
may grow too large, we'll break it into smaller pieces when needed. For now, we'll consolidate everything there.
- While
Edit - another bit of words to help cast the vision
*.cy.ts - the test files, define the list of steps we want to accomplish in a test. For tests that go down the create new path, we define the implementation of how cypress goes about acting or verifying in createNew. We define an action or a verification 1 time and then reuse it by including its step name in our list of steps. These are enums and not bare strings so that your IDE will prompt you with existing steps.
@@ -9,10 +9,10 @@ import { UniversalActions } from '../support/universalActions' | |||
describe('The Redesigned Create Protocol Landing Page', () => { | |||
beforeEach(() => { | |||
cy.visit('/') | |||
cy.contains('button', 'Confirm').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to a new command in protocol-designer/cypress/support/commands.ts
and give it a meaningful name.
}) | ||
|
||
it('successfully loads a protocol exported on a previous version', () => { | ||
cy.contains('button', 'Confirm').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this in the beforeEach
using the new command you create.
const protocol = getTestFile(TestFilePath.DoItAllV7) | ||
cy.importProtocol(protocol.path) | ||
verifyOldProtocolModal() | ||
verifyImportProtocolPage(protocol) | ||
}) | ||
|
||
it('successfully loads a protocol exported on the current version', () => { | ||
cy.contains('button', 'Confirm').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -6,7 +6,9 @@ import { TestFilePath } from '../support/testFiles' | |||
describe('Protocol fixtures migrate and match snapshots', () => { | |||
beforeEach(() => { | |||
cy.visit('/') | |||
cy.closeAnalyticsModal() | |||
// Get rid of the analytics | |||
cy.contains('button', 'Confirm').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the command.
@@ -1,7 +1,7 @@ | |||
describe('The Settings Page', () => { | |||
before(() => { | |||
cy.visit('/') | |||
cy.closeAnalyticsModal() | |||
cy.contains('button', 'Confirm').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the command.
@@ -19,6 +18,7 @@ declare global { | |||
left_pipette_selector: string, | |||
right_pipette_selector: string | |||
) => Cypress.Chainable<void> | |||
chooseDeckSlot: (slot: string) => Cypress.Chainable<void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove and put in createNew
@@ -101,6 +104,8 @@ Cypress.Commands.add('verifyCreateNewHeader', () => { | |||
|
|||
// Home Page | |||
Cypress.Commands.add('verifyHomePage', () => { | |||
// Todo re-add when Once 8.2.2 comes back in | |||
cy.contains('button', 'Confirm').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you have the new command created, use it here.
// cy.get('[data-testid="SettingsIconButton"]').click(); | ||
cy.getByTestId(locators.settingsDataTestid).click() | ||
// ToDo re-add when 8.2.2 pushed to edge | ||
// cy.get('[data-testid="analyticsToggle"] svg') | ||
// .should('have.css', 'fill', 'rgb(0, 108, 250)') | ||
cy.getByTestId(locators.settingsDataTestid).click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command should have one job and that is to click the Create New button. Handling other blocking steps should be done separately.
Cypress.Commands.add('robotSelection', (robotName: string) => { | ||
if (robotName === 'Opentrons OT-2') { | ||
cy.contains('label', locators.OT2_Home).should('be.visible').click() | ||
} else { | ||
// Just checking that the selection modal works | ||
cy.contains('label', locators.OT2_Home).should('be.visible').click() | ||
cy.contains('label', locators.Flex_Home).should('be.visible').click() | ||
} | ||
cy.contains('button', 'Confirm').should('be.visible').click() | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already in createNew
Cypress.Commands.add('chooseDeckSlot', (slot: string) => { | ||
// Define the deckSlots object with a Record type for valid keys | ||
const deckSlots: Record< | ||
| 'A1' | ||
| 'A2' | ||
| 'A3' | ||
| 'B1' | ||
| 'B2' | ||
| 'B3' | ||
| 'C1' | ||
| 'C2' | ||
| 'C3' | ||
| 'D1' | ||
| 'D2' | ||
| 'D3', | ||
() => Cypress.Chainable<JQuery<HTMLElement>> | ||
> = { | ||
A1: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'), | ||
A2: () => cy.contains('foreignObject[x="164"][y="321"]', 'Edit slot'), | ||
A3: () => cy.contains('foreignObject[x="328"][y="321"]', 'Edit slot'), | ||
B1: () => cy.contains('foreignObject[x="0"][y="214"]', 'Edit slot'), | ||
B2: () => cy.contains('foreignObject[x="164"][y="214"]', 'Edit slot'), | ||
B3: () => cy.contains('foreignObject[x="328"][y="214"]', 'Edit slot'), | ||
C1: () => cy.contains('foreignObject[x="0"][y="107"]', 'Edit slot'), | ||
C2: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'), | ||
C3: () => cy.contains('foreignObject[x="328"][y="107"]', 'Edit slot'), | ||
D1: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'), | ||
D2: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'), | ||
D3: () => cy.contains('foreignObject[x="328"][y="0"]', 'Edit slot'), | ||
} | ||
|
||
// Type-safe slot action assignment | ||
const slotAction = deckSlots[slot as keyof typeof deckSlots] | ||
|
||
// Check if slotAction exists and call it, else throw an error | ||
if (slotAction) { | ||
slotAction() // Execute the Cypress command for the selected slot | ||
} else { | ||
throw new Error(`Slot ${slot} not found in deck slots.`) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to createNew
Overview
These additions to the test case of transferSetting.js.cy are the start of providing greater test coverage to our transfer form.
At this moment, it only goes up to saving a liquid and then opening the transfer form.
Test Plan and Hands on Testing
These are changes exclusively to cypress. So it will be tested when folks run their code.
Changelog
Please add special attention to checking if this was the correct way to add liquid. Saving it took a bit of extra effort. AFAIK
So I tried to do the following
Review requests
@y3rsh @jerader
Risk assessment
Medium. This might take some extra time with developers to ensure that I'm not breaking existing testing behavior.